Skip to content

Conversation

@4yman-0
Copy link

@4yman-0 4yman-0 commented Jan 1, 2025

Fix #2754

ImprovedTube.chapters tries to call ImprovedTube.forbidFocus which does not exists or is not a function. Tested with Chromium on Arch Linux and random youtube videos.

Fix code-charity#2754

`ImprovedTube.chapters` tries to call `ImprovedTube.forbidFocus` which does not exists or is not a function.
Tested with Chromium on Arch Linuxand random youtube videos.
@4yman-0 4yman-0 requested a review from ImprovedTube January 1, 2025 07:26
@4yman-0

This comment was marked as off-topic.

@ImprovedTube
Copy link
Member

thank you! @4yman-0 glad you are here! sorry this one seems to be a duplicate of #2750

@4yman-0

This comment was marked as resolved.

@ImprovedTube
Copy link
Member

ImprovedTube commented Jan 6, 2025

hi @4yman-0, i appreciated your thoughts in the code.
the bug #2754 was undone in #2750.

While defining a function for every user in every YouTube-tab is cheap,
i wanted to make it conditional (it just appears in 3 features yet.)
So i had defined it conditionally in functions.js:

if (document.documentElement.dataset.pageType === 'video'
	 && (ImprovedTube.storage.description === "expanded" || ImprovedTube.storage.transcript === true || ImprovedTube.storage.chapters === true )) { ImprovedTube.forbidFocus =  function (ms) }

however i undid this to undo the bug

if you like, we should check why, going back to Dec21, https://github.com/code-charity/youtube/tree/HEAD@%7B2024-12-21%7D, typos? timing?/#2415

@4yman-0
Copy link
Author

4yman-0 commented Jan 6, 2025

So i defined it conditionally in functions.js:

if (document.documentElement.dataset.pageType === 'video'
	 && (ImprovedTube.storage.description === "expanded" || ImprovedTube.storage.transcript === true || ImprovedTube.storage.chapters === true )) { ImprovedTube.forbidFocus =  function (ms) }

I'll close the pull request if that's the case :)

@4yman-0 4yman-0 removed the request for review from ImprovedTube January 6, 2025 12:15
@4yman-0 4yman-0 self-assigned this Jan 6, 2025
@ImprovedTube
Copy link
Member

ImprovedTube commented Jan 6, 2025

hi! :) if or when? you chose, i'm fine if this thread keeps evolving - or to hoard open PRs for decades

Or i can merge for our motivation (and remove the function from the other two features where it appears)

@ImprovedTube
Copy link
Member

Great team-work! 👍🎉

@ImprovedTube ImprovedTube self-assigned this Jan 6, 2025
@4yman-0
Copy link
Author

4yman-0 commented Jan 6, 2025

This PR sets forbidFocus initialy while your commits make improvedTube.chapters set forbidFocus every time it is called.

Can you review?

@ImprovedTube ImprovedTube changed the title Fix Chapters not showing after reload Edit: Defining forbidFocus() once in advance, instead of inside features Jan 7, 2025
@ImprovedTube
Copy link
Member

we should check why, going back to Dec21, https://github.com/code-charity/youtube/tree/HEAD@%7B2024-12-21%7D, typos? timing?/#2415

/why the now out-commented condition might not yet work at that point in the code

Copy link
Author

@4yman-0 4yman-0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks Good To Me!

@ImprovedTube ImprovedTube added the untested please test. (also applies to proactively merged pull requests.) label Jan 7, 2025
@4yman-0
Copy link
Author

4yman-0 commented Jan 24, 2025

I tested it today. example video with chapters from YouTube works fine. but the back/forward buttons hide the sidebar without any errors.

Edit: Resizing the width of the browser window shows the sidebar again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

untested please test. (also applies to proactively merged pull requests.)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🐞Chapters not show in sidebar

2 participants